Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4d2dd5a) 84.53% compared to head (bf73059) 84.53%.
Report is 2 commits behind head on main.

❗ Current head bf73059 differs from pull request most recent head 511da8f. Consider uploading reports for the commit 511da8f to get more accurate results

@@           Coverage Diff           @@
##             main    #1764   +/-   ##
=======================================
  Coverage   84.53%   84.53%           
=======================================
  Files         307      307           
  Lines        6777     6777           
  Branches     1043     1043           
=======================================
  Hits         5729     5729           
  Misses        839      839           
  Partials      209      209           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Does it make sense to have a "standard"/"classic" section under the V7 paragraph?

I think we should keep it for the reasons you mentioned - note that v8 does not have it anymore.

Shall I add a note at the end of this circuit breaker migration paragraph?

I think this is fine, another alternative is to use the low-allocation ExecuteOutcomeAsync API to avoid throwing exception. In that case caller can just execute the callback that will fail very quickly with minimum overhead.

I think this is fine, another alternative is to use the low-allocation ExecuteOutcomeAsync API to avoid throwing exception. In that case caller can just execute the callback that will fail very quickly with minimum overhead.

I think it might make sense to mention this case on the migration guide and provide a link to the related anti-pattern section where we detail the solution. Do you agree?

I think this is fine, another alternative is to use the low-allocation ExecuteOutcomeAsync API to avoid throwing exception. In that case caller can just execute the callback that will fail very quickly with minimum overhead.

I try to put together the suggested way but it feels a bit clumsy for me.

The anti-pattern

var stateProvider = new CircuitBreakerStateProvider();
var circuitBreaker = new ResiliencePipelineBuilder()
    .AddCircuitBreaker(new()
    {
        ShouldHandle = new PredicateBuilder().Handle<HttpRequestException>(),
        BreakDuration = TimeSpan.FromSeconds(0.5),
        StateProvider = stateProvider
    })
    .Build();

if (stateProvider.CircuitState
    is not CircuitState.Open
    and not CircuitState.Isolated)
{
    await circuitBreaker.ExecuteAsync(static ct =>
    {
        // Your code goes here
        return default;
    }, CancellationToken.None);
}

The pattern

var context = ResilienceContextPool.Shared.Get();
var circuitBreaker = new ResiliencePipelineBuilder()
    .AddCircuitBreaker(new()
    {
        ShouldHandle = new PredicateBuilder().Handle<HttpRequestException>(),
        BreakDuration = TimeSpan.FromSeconds(0.5),
    })
    .Build();

Outcome<bool> result = await circuitBreaker.ExecuteOutcomeAsync(static (ctx, state) =>
{
    // Your code goes here

    return Outcome.FromResultAsValueTask(true);
}, context, "state");

ResilienceContextPool.Shared.Return(context);

Using bool as a TResult feels a bit odd. Even if I replace that to object? still does not feel good

Outcome<object?> result = await circuitBreaker.ExecuteOutcomeAsync(static (ctx, state) =>
{
    // Your code goes here

    return Outcome.FromResultAsValueTask<object?>(null);
}, context, "state");

@martintmk Do you have any other alternative which feels more natural?

@peter-csala

The main idea is to just execute the callback as one normally would and afterwards one can check the result. For example:

Outcome<bool> result = await circuitBreaker.ExecuteOutcomeAsync(static (ctx, state) => ... );

if (result.Exception is BrokenCircuitException)
{
   // the execution was stopped by CB
}
else
{
   // the callback was executed and passed through CB
}

@martintmk Yeah I do understand that part. Let me try to rephrase my problem.

There is no non-generic Outcome data structure. That's why using ExecuteOutcomeAsync does not neatly work well with async methods.

If there were a non-generic Outcome then you would write something like this

Outcome result = await circuitBreaker.ExecuteOutcomeAsync(static (ctx, state) =>
{
    // Your code goes here

    return ValueTask.FromResult(new Outcome());
}, context, "state");

Anyway maybe overthinking and it is the same problem as with the TaskCompletionSource<TResult>.

@martintmk Yeah I do understand that part. Let me try to rephrase my problem.

There is no non-generic Outcome data structure. That's why using ExecuteOutcomeAsync does not neatly work well with async methods.

If there were a non-generic Outcome then you would write something like this

Outcome result = await circuitBreaker.ExecuteOutcomeAsync(static (ctx, state) =>
{
    // Your code goes here

    return ValueTask.FromResult(new Outcome());
}, context, "state");

Anyway maybe overthinking and it is the same problem as with the TaskCompletionSource<TResult>.

You are right, there is currently no way to return "void outcome", you need to use workaround you mentioned above. Still, this is for more advanced scenarios so I would wait for feedback until someone really complains :)

You are right, there is currently no way to return "void outcome", you need to use workaround you mentioned above. Still, this is for more advanced scenarios so I would wait for feedback until someone really complains :)

In this case I will use HttpResponseMessage as TResult to make the example smooth.